Conversation
…elf-service (SID-AUTH-06) Implements #195 on the WalletInstance model instead of a parallel per-passkey status, superseding #196. - WalletLifecycleService: validated transitions, audit, and the cascade a deactivation implies. Suspension is reversible and only blocks; revocation is terminal; revoking the last non-revoked instance deactivates the wallet: private data, server-side credentials/presentations and pending challenges are erased, live sessions dropped. Data is never erased while an instance the user could still reactivate remains. The user record and passkeys stay so the refusal is attributable ("wallet revoked", not "user not found"). - Login gate in WebAuthnService.FinishLogin: the instance linked to the passkey must be active; a wallet with every instance revoked refuses every passkey. Both login handlers (legacy and AS) return 403 WALLET_SUSPENDED / WALLET_REVOKED. - Passkey link: WIA generate accepts an optional credential_id, recorded as WalletInstance.CredentialID (the field existed but nothing set it). - Self-service endpoints under /user/session/instances: list, change status of own instance, revoke-all. Admin status changes go through the same service when wired, so both paths share one cascade. - Engine session store wired into the lifecycle service (main.go). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The login handler currently returns a misleading “wallet deactivated / re-enroll” message for revocation cases where only a single instance is revoked, and the wallet erasure path silently falls back to default-tenant cleanup without warning when tenant membership lookup fails.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR implements SID-AUTH-06 wallet instance lifecycle management on top of the existing WalletInstance model, adding validated suspend/revoke transitions with a deactivation (erasure) cascade, plus login gating and self-service/admin APIs to manage instances.
Changes:
- Introduces
WalletLifecycleServiceto centralize lifecycle transitions, auditing, session dropping, and “last-instance revoked” wallet-data erasure. - Adds optional
credential_idplumbing (WIA generate → instance store) to link a passkey to a wallet instance for per-device login refusal. - Enforces lifecycle state at login (post-assertion verification) and exposes self-service instance management endpoints; wires admin status changes through the same lifecycle service when available.
File summaries
| File | Description |
|---|---|
| internal/storage/mongodb/wallet_instance.go | Persist credential_id on upsert without changing status for existing instances. |
| internal/storage/memory/wallet_instance.go | Mirror credential_id persistence behavior in memory store. |
| internal/storage/memory/wallet_instance_test.go | Adds regression test ensuring upsert records credential_id without reactivating status. |
| internal/service/wua_status_claims_test.go | Updates tests for signWIA signature change. |
| internal/service/wia.go | Extends WIA request to accept credential_id and records it onto wallet instances. |
| internal/service/webauthn.go | Adds SID-AUTH-06 login gate enforcing instance/wallet lifecycle at authentication. |
| internal/service/webauthn_lifecycle_test.go | Unit tests for login-gate lifecycle enforcement behavior. |
| internal/service/wallet_lifecycle.go | New lifecycle service implementing transitions, audit emission, session drop, and erasure cascade. |
| internal/service/wallet_lifecycle_test.go | Unit tests for suspend/revoke, revoke-all, ownership, terminal transitions, and erasure behavior. |
| internal/service/services.go | Registers WalletLifecycleService in the service container. |
| internal/server/providers.go | Wires self-service routes and injects lifecycle service into admin handlers when available. |
| internal/as/passkey.go | Maps lifecycle refusal errors to stable 403 codes in AS passkey finish handler. |
| internal/as/passkey_test.go | Tests 403 lifecycle refusal mapping for AS passkey finish handler. |
| internal/api/wia_handlers.go | Adds credential_id field to WIA generate API request. |
| internal/api/instance_handlers.go | New self-service endpoints for listing/updating/revoking instances via lifecycle service. |
| internal/api/instance_handlers_test.go | Tests self-service instance lifecycle endpoints, ownership behavior, and terminal states. |
| internal/api/handlers.go | Maps lifecycle refusal errors to 403 responses in standard login finish handler. |
| internal/api/admin_instance_handlers.go | Routes admin status changes through lifecycle service (with cascade) when configured. |
| internal/api/admin_instance_handlers_test.go | Tests admin lifecycle cascade behavior when lifecycle service is wired. |
| internal/api/admin_handlers.go | Adds lifecycle service wiring hook to admin handlers. |
| docs/API.md | Documents wallet instance lifecycle semantics and self-service endpoints. |
| cmd/server/main.go | Wires session store into lifecycle service so suspend/revoke drops live sessions. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Sep 8, 2026
…ce; log tenant lookup failures on erasure Review follow-ups on the SID-AUTH-06 login gate: - Add ErrWalletDeactivated (wraps ErrWalletInstanceRevoked) for the "every instance revoked" case. The login handler keeps the stable WALLET_REVOKED code but the message now says whether other enrolled devices still work or the wallet is gone and must be re-enrolled. Previously the "deactivated; re-enroll" message was shown for a single revoked instance. - eraseWalletData logs at error level when the tenant membership lookup fails (only the default tenant is then erased), instead of silently treating a failure like an empty membership. - Tests for both cases and for audit emission per transition; API.md documents the two messages. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMxP45ngJW44ju9qXqsVvJ
…min error strings Sonar quality gate follow-ups for the SID-AUTH-06 PR: - eraseWalletData split into tenantsForErasure and eraseHolderData (cognitive complexity 19 -> under the limit). tenantsForErasure now always includes the default tenant, where users registered without an explicit membership row keep their data, so a user with an explicit membership elsewhere no longer leaves default-tenant credentials behind on deactivation. - Admin instance handler error strings are constants (S1192). - Tests for every logged-and-continue branch of the erasure cascade, the stop conditions (user unloadable, remaining instances unlistable), the handler 401/503/500 paths and the admin lifecycle path's storage failure. New-code coverage goes from ~70% to ~90%. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMxP45ngJW44ju9qXqsVvJ
Contributor
Author
|
Review pass 2026-09-14 (@smncd, for when you pick this up):
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical lifecycle, erasure, login-gating, credential-binding, and session-invalidation issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
internal/api/instance_handlers.go:104
- Ignoring
ShouldBindJSONerrors means malformed non-empty bodies are treated like an empty optional body. For example,{still revokes every instance with an empty reason and returns 200. Keep the body optional for EOF, but return 400 for other JSON decoding errors before performing this destructive operation.
internal/service/wallet_lifecycle.go:109
RevokeAllForUserenumerates only the instances intenantID, although the self-service endpoint is documented as deactivating the wallet and the subsequent erasure is global. For a multi-tenant user, instances in another tenant remain active and can still obtain WIAs after the wallet data has been erased. Revoke all of the user's instances across tenant memberships, or explicitly make this API tenant-scoped and avoid global deactivation here.
instances, err := s.ListForUser(ctx, tenantID, userID)
internal/service/wallet_lifecycle.go:146
- This global deactivation check only lists instances in the current tenant, but
eraseWalletDatathen removes the user's data from every tenant. If the user has an active or suspended instance in another tenant, revoking the last instance here can erase the wallet while that other instance remains usable/reactivatable. Check for non-revoked instances across all of the user's tenants before performing global erasure, or make the erasure tenant-scoped.
remaining, err := s.store.WalletInstances().GetByUser(ctx, tenantID, userID)
internal/service/wallet_lifecycle.go:128
- If a previous revocation changed all statuses but
cascadefailed part-way, a retry ofrevoke-allsees every instance already revoked, leaveslast == nil, and skips cleanup entirely. Run the idempotent cascade whenever the user has instances and they are all revoked, even whenchanged == 0, so a transient erasure failure can be repaired.
if last != nil {
s.cascade(ctx, tenantID, last)
internal/storage/memory/wallet_instance.go:38
- The in-memory store has the same link-replacement behavior as the Mongo store: every later non-empty
CredentialIDoverwrites the established passkey link. That lets tests and development deployments diverge from an immutable 1:1 instance-to-passkey association and can let the original passkey bypass a suspension. Only fill the link when it is currently empty, matching the intended first-attestation association.
if instance.CredentialID != "" {
existing.CredentialID = instance.CredentialID
}
- Files reviewed: 26/26 changed files
- Comments generated: 9
- Review effort level: Lite
leifj
added a commit
that referenced
this pull request
Sep 14, 2026
Review follow-ups on #325: - cmd/server: the session-cleaner wiring lived inside the RoleEngine branch, so a plain --mode=backend process never gave UserService a cleaner and deleting a user left its AS cookie sessions valid. Wire the AS cleaner whenever the backend provider exists and add the engine session store only when the engine runs in this process. - as.SessionStore.DeleteByUser: narrow the doc comment to what is wired today (UserService.DeleteUser). Wallet-instance suspension/revocation is added by #319 and gets the same cleaner when the two land. - Add unit tests for service.MultiSessionCleaner (fan-out, nil entries, first-error semantics) - the only new non-Mongo lines Sonar reported as uncovered. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMxP45ngJW44ju9qXqsVvJ
… and WIA guard Copilot review follow-ups on #319: - eraseWalletData clears User.Keys (the legacy key blob uploaded at registration) along with PrivateData: both are key material. - Credentials and presentations are stored under the holder DID, which the API handlers (getHolderDID) fall back to the user id for users without a DID. Erasure now uses the same fallback instead of skipping holder data when User.DID is empty. - checkWalletLifecycle decides deactivation before mapping the linked instance's status: when every instance is revoked, a passkey linked to one of them is refused with ErrWalletDeactivated, not with ErrWalletInstanceRevoked ("use another device") - no device can log in any more. - GenerateWIA's lifecycle guard only looked at the presented JKT, so an access token that outlived deactivation could attest a brand-new key and record a fresh active instance, re-opening login without a new enrollment. A first attestation now refuses when every instance of the user is revoked (ErrWIAInstanceDeactivated); a suspended, reactivatable instance still allows enrolling another device. Tests cover each case. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMxP45ngJW44ju9qXqsVvJ
Sonar S3776 flagged GenerateWIA at cognitive complexity 18 after the deactivated-wallet guard was added in e0c640b. The helper now takes the *domain.UserID and returns nil for anonymous attestations, so the call site loses one nesting level. No behaviour change; existing tests cover both the nil-user and the deactivated paths. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMxP45ngJW44ju9qXqsVvJ
WalletInstance.CredentialID is supplied by the client at WIA generation and was overwritten on every re-attestation, so a later attestation could move the instance to another (or a nonexistent) passkey and the original passkey would no longer match checkWalletLifecycle's per-instance suspend/revoke gating. Both stores now record only the first non-empty link: the memory store checks the existing record, the MongoDB store does a second UpdateOne whose filter matches the document only while credential_id is absent or empty, so "first link wins" is atomic. A missing link can still be filled in by a later attestation. The contract is documented on storage.WalletInstanceStore.Upsert and covered by tests in both stores. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMxP45ngJW44ju9qXqsVvJ
This was referenced Sep 14, 2026
… and are retryable Design feedback on #319: - Wallet instances are per tenant. Revoking the last non-revoked instance of a user in a tenant erases the credentials and presentations held in that tenant only. The user-level data shared across tenants (encrypted private data, legacy key blob, pending challenges) is erased once no non-revoked instance remains in any tenant the user belongs to (memberships plus the default tenant); a failed membership lookup keeps it, since erasing on a guess could destroy a wallet still in use. The login gate stays tenant-scoped, matching the decision. - Cascade failures are no longer swallowed. The status change is persisted first and stands; if dropping sessions or erasing data then fails, ChangeStatus/RevokeAllForUser return ErrErasureIncomplete (wrapping the underlying errors) together with the persisted state, and the handlers answer 409 ERASURE_INCOMPLETE. Repeating the same request on an already revoked instance (or revoke-all with nothing left to revoke) re-runs the cascade, so the client retries until 200. Tests cover both tenants' data, the vault decision, every failing store operation with a successful retry, and the 409 mapping in the self-service and admin handlers. API.md documents the semantics. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMxP45ngJW44ju9qXqsVvJ
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved lifecycle integrity and destructive-operation issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
internal/service/wallet_lifecycle.go:227
- Clearing the vault here is not durable against already-issued bearer tokens: this service only drops the wired engine sessions, while protected private-data and credential endpoints authorize from
user_id, and legacyAuthMiddlewareaccepts unexpired JWTs. A token issued before revocation can therefore write new private data or credentials after this cascade, defeating secure erasure and the re-enrollment gate. Invalidate all access tokens or enforce the deactivated-wallet state on every wallet-mutating authenticated path.
user.PrivateData = nil
user.PrivateDataETag = ""
user.Keys = nil
internal/service/wia.go:618
CredentialIDis now part of the login-enforcement boundary, butsignWIAsigns and returns the WIA even when the laterWalletInstanceStore.Upsert(including the Mongo link update) fails; it only logs that error. A transient persistence failure can therefore issue a valid WIA for an unlinked instance, so later suspension/revocation will not gate that passkey. Persist the instance/link successfully before returning the token, or fail the request so the client retries.
CredentialID: credentialID,
internal/storage/mongodb/wallet_instance.go:65
- This second write is the only persistence of a supplied passkey link, but
WIAService.signWIAlogsUpserterrors and still returns the WIA. If the base upsert succeeds and this link update fails, the client receives a usable WIA while the instance remains unlinked, so a later per-instance suspend/revoke cannot block that passkey. Make the link write atomic with the upsert or fail WIA issuance when a supplied link cannot be persisted.
if _, err := s.collection.UpdateOne(ctx, linkFilter, bson.M{"$set": bson.M{"credential_id": instance.CredentialID}}); err != nil {
return fmt.Errorf("%w: link wallet instance credential: %v", storage.ErrDatabase, err)
- Files reviewed: 30/30 changed files
- Comments generated: 6
- Review effort level: Lite
Comment on lines
+101
to
+104
| if h.lifecycle != nil { | ||
| // Shared lifecycle service: same transition rules, audit and cascade | ||
| // (session drop, wallet erasure on last revocation) as self-service. | ||
| if _, err := h.lifecycle.ChangeStatus(c.Request.Context(), service.LifecycleActor{Kind: "provider"}, tenantID, instanceID, status, req.Reason); err != nil { |
Comment on lines
+225
to
+229
| user.PrivateData = nil | ||
| user.PrivateDataETag = "" | ||
| user.Keys = nil | ||
| user.UpdatedAt = time.Now() | ||
| if err := s.store.Users().Update(ctx, user); err != nil { |
Comment on lines
+346
to
+347
| if err := s.refuseIfWalletDeactivated(ctx, tenantID, userID); err != nil { | ||
| return "", err |
Comment on lines
+657
to
+658
| if len(instances) == 0 { | ||
| return nil |
…plicate passkey links Two findings from the Copilot re-review of a6e86e5: - POST /user/session/instances/revoke-all discarded JSON binding errors so that the body could be omitted, which also let a malformed non-empty body (a truncated reason, say) revoke every instance unannotated. The handler now accepts only an empty body as "no options" and answers 400 for any other decoding error, without touching the instances. - checkWalletLifecycle picked the first instance linked to the presenting passkey, so when the same CredentialID was linked to both a revoked (or suspended) and an active instance, store ordering decided whether the passkey could log in. Every linked instance is now considered and the most restrictive status wins. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HMxP45ngJW44ju9qXqsVvJ
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements #195 (SID-AUTH-06, tracking sirosfoundation/compliance#78) on top of the existing
WalletInstancemodel, and supersedes #196, which put a second, per-passkeydeactivatedstatus next to it.@smncd requesting your review: this changes login behaviour and adds an erasure cascade.
Why not #196
Main grew a first-class wallet instance model after #196 was written: instances keyed by instance-key thumbprint,
active/suspended/revokedwith validated transitions, admin endpoints, audit events, and enforcement at WIA generation. #196 would have added a disconnected passkey-level lifecycle beside it, had no reversible state, and wiped the user's private data on lockout, which is irreversible and shared across the user's devices.What this adds
WalletLifecycleService(internal/service/wallet_lifecycle.go): validated transitions, audit, and the cascade. Suspension is reversible and only blocks. Revocation is terminal. Revoking the last non-revoked instance deactivates the wallet: private data, server-side credentials/presentations and pending challenges are erased and live sessions dropped, so re-activation requires a full new enrollment. Nothing is erased while a suspended (reactivatable) instance remains. The user record and passkeys stay so login can be refused with a clear reason.FinishLogin: the instance linked to the passkey must be active; a wallet with every instance revoked refuses every passkey. Checked only after the assertion verified. Both login handlers return403 WALLET_SUSPENDED/WALLET_REVOKED.POST /wallet-provider/wia/generateaccepts optionalcredential_id, recorded asWalletInstance.CredentialID. That field was documented but nothing ever set it. Without it the gate still enforces whole-wallet deactivation; with it, per-device suspension also blocks that device's login./user/session/instances:GETlist,PUT {id}/status,POST revoke-all. Ownership is checked; other users' instances read as 404.main.go.Client follow-ups (not in this PR)
credential_idat WIA generation, and show the instance list / offer suspend and deactivate (wallet-frontend#148 needs re-scoping from the Implement wallet credential lifecycle deactivation and enforcement (SID-AUTH-06) #196 API to this one).Tests
Service: suspend blocks without erasing and can be reversed; revoking the last instance erases, an earlier one does not; revoke-all; ownership, tenant and terminal-state rules. Login gate: no instances, linked suspended/revoked, unlinked suspended (does not block other passkeys), all revoked. Handlers: self-service list/update/not-owned/bad status/revoke-all/terminal; admin cascade through the lifecycle service; AS and passkey 403 mapping; memory store records the passkey link without touching status. Full
go test ./...andgolangci-lintclean.🤖 Generated with Claude Code